Allow updating configuration of changes tries #3201

svyatonik · 2019-07-25T07:30:14Z

IMPORTANT NOTES FOR REVIEWERS

this PR adds column to the database, but provides no upgrade functionality. I've checked that last time we changed this constant, there were also no upgrade. But it has been ~4 months ago, so probably we need to implement auto-upgrade now?
this PR should change absolutely nothing when chain is configured to work without changes tries (which is by default). That's (IMO) the main thing to look at in this PR

=================================================================

This PR introduces new changes-tries related digest item (ChangesTrieSignal) + separate SRML method that is used to change CT configuration (and emits this signal). It is also possible to use the same signal for emergency creation of max-level digest CT, though it'll be implemented in next PRs.

The idea is to use consensus cache (which from now shouldn't be called consensus cache anymore) to hold intervals (and also config itself) where CT configuration hasn't be changed. When building CT, you only need actual CT configuration => work is done within single interval. When fetching key changes/proof, this may affect several intervals => every interval will have its own state_machine/src/changes_trie/* related call.

Detailed description:

(extracted to separate PR) cache is modified to support pruning strategy selection - we do not actually need authorities once block is finalized (so cached entries can be pruned). But we may need to hold changes tries configuration forever. So there are two strategies: NeverPrune and ByDepth(N);
(extracted to separate PR) since cache is now not consensus-specific, I've moved well_known_cache_keys from consensus_common to the client::backend;
moved db-backed changes trie storage (DbChangesTrieStorage) from the db/src/lib.rs into dedicated file db/src/changes_tries_storage.rs. Now it holds reference to the cache where configuration changes are stored;
(extracted to separate PR) state_machine methods now accept new struct ChangesTrieState instead of simply ChangesTrieStorage. In addition to storage reference, it holds: active changes trie configuration and zero block of this configuration (i.e. parent block of the block where first changes trie is built using this configuration). This is required to build changes trie at current block;
Client::key_changes and Client::key_changes_proof are updated to support ranges where CT configuration has been changed. Client::max_key_changes_range isn't updated yet, because it tightly related to pruning (see below);
(extracted to separate PR) SurfaceIterator is extracted to dedicated state_machine/src/changes_trie/surface_iterator.rs;
(extracted to separate PR) when changes trie configuration change is detected inside the build.rs, max level digest is created. It may be a skewed digest (the digest that is created ahead of schedule), or a normal digest.
key changes functions are updated to support skewed digests.

Changes tries are pruned similarly to what has been before - i.e. there's a block for which we guarantee that all changes tries (if created) after this block are valid (i.e. if we prune some trie, then the digest trie that points to it, is also pruned) and all changes tries before this block should not be accessed. Since we now have tries that are built using different configurations, we store this block in special metadata entry (because it isn't that easy to calculate it).

svyatonik · 2020-01-09T09:57:34Z

Duplicating IMPORTANT NOTE here: please only merge this PR if you sure that we do not need code for db upgrade here. I haven't did that - not sure if it is required. If merged as-is, this PR will break all existing databases => nodes will be unable to start with old databases. Please respond if you think we should start database versioning stuff.

gavofyork · 2020-01-10T00:48:52Z

will this affect typical deployments (like kusama nodes)?

svyatonik · 2020-01-10T06:14:38Z

If you're talking about breaking db - yes, this will affect all nodes. So you can't just copy binary over previous - we need to erase db && then (if there's another source of blocks on the network), it'll sync from the scratch.

Demi-Marie · 2020-01-10T14:58:01Z

@svyatonik will nodes with this patch be able to sync from nodes without it?

svyatonik · 2020-01-10T15:09:35Z

@demimarie-parity Yes, it changes nothing in how blocks on existing chains are processed.

gavofyork · 2020-01-13T08:20:47Z

If you're talking about breaking db - yes, this will affect all nodes. So you can't just copy binary over previous - we need to erase db && then (if there's another source of blocks on the network), it'll sync from the scratch.

how much trouble would a DB migration be? we have a semi-production network of ~500 nodes out there and i don't really want to have to force them all to resync on the next minor revision..

svyatonik · 2020-01-13T08:26:47Z

Re upgrade performance - I'm not sure yet. Re code - shouldn't be a big issue, I just thought that maybe we do not need it yet. We have changed db format several times, but there never been any upgrade code. But it was probably been before Kusama. So I'll add db upgrade here. Will try to keep it localized in single commit to make it easy reviewable.

svyatonik · 2020-01-13T08:27:46Z

(or perhaps will open additional PR to the changes_tries_update_configuration branch - I believe set of reviewers should be different)

gavofyork · 2020-01-13T11:16:26Z

this one shouldn't be merged without migration code, so probably best if it's a single commit on top of this.

…e_configuration

svyatonik · 2020-01-14T13:12:28Z

This commit implements database upgrade. The only upgrade now is from v0 to v1, which: (1) adds new column to the database and (2) initializes changes tries configuration cache (with None configuration).

I also forgot that I've promised to sync Kusama with these changes in some of comments above => will temporary switch to inprogress and revert will try to sync.

cheme · 2020-01-15T08:55:59Z

Had a quick peak on the db commit. Things looks good (considering meta column is the same between light and full).
Makes me think about a possible change (not sure it is needed but anyway it can be added later):

here new client instance will apply all upgrade in sequence. We could also have kvdb-rocksdb with a new function to check if db already exists and add a latest version file on creation (by default only run upgrade on client with existing db).
previous point does make sense but it is also a possible cause for error (if upgrade code does not match initialization code), so it is only interesting if we can manage db version per client (eg at this point polkadot version 0 corresponding to kusama version 1). So the versioning and upgrade code would be define per project, probably putting thing being an upgradedb trait and including the impl in cli initialisation parameters.

svyatonik · 2020-01-15T09:15:52Z

Thanks for taking a time to review! I agree that function like db.exists() would be much better than the current approach. Probably we could even use directory.exists() here. But that could be added on next upgrade. For now I would prefer to stick with current version. The reason is simple: (1) now we have only one upgrade which works in both cases (when db_version exists and contains "0" and when it does not exists) (2) I hope most of nodes will upgrade to v1 (and db_version will be created with "1") until next upgrade (v2) will be required. This way we could minimize number of nodes that are upgrading from "either v0, or v2" to "v2" - in most cases we'll either have "v1", or "v2" (i.e. there'll (almost) be no nodes without db_version file).

As for the second - I'm not sure we are exposing anything internal from database to clients. The only exception is aux storage, but it already has its own upgrade path - see e.g. upgrade_from_* here. So I don't think there's a need for providing different upgrade scenarios for different projects. I'd say projects should use dedicated storage for their own needs.

svyatonik · 2020-01-16T07:19:59Z

So I've finally synced both Kusama clients (full/light) with this PR - everything seems fine for me.

svyatonik added 15 commits June 25, 2019 10:18

DigestItem::ChangesTrieSignal

611f481

introduce changes_trie::State

8b65b04

introduce config activation block

0cd05df

ChangesTrieSignal::as_new_configuration

40ed2c2

moved well_known_cache_keys to client

c01ecd6

extracted DbChangesTrieStorage to separate file

73f1933

change meaning of none in blockchain cache

315303a

changes trie config (FULL) cache draft

67642d1

eliminating const ChangesTrieConfiguration

a7e240d

delay pruning

5256d03

continue elimination

698e2ff

do not prune CT config from cache

04a8fa0

removed redundant code

afda822

fix some TODOs

e08183d

Merge branch 'master' into changes_tries_update_configuration

368563f

svyatonik added A3-in_progress Pull request is in progress. No review needed at this stage. M4-core labels Jul 25, 2019

svyatonik added 13 commits July 29, 2019 09:39

introduce ConfigurationRange

01e3792

use Configuration range in build

c1913fc

build skewed digest

fb6ae9c

remove debug print

e429061

extracted surface iterator

e6acae8

key_changes works with skewed digests

68527fb

fix client build

5a839e5

add test for NeverPrune

9729912

fix TODO

bca439a

fixed some TODOs

383795a

more tests

f6993f6

fixing TODOs

8b70450

fixed compilation

1b3b565

svyatonik added A0-please_review Pull request needs code review. and removed A5-grumble labels Jan 9, 2020

gavofyork added the A1-onice label Jan 13, 2020

Merge remote-tracking branch 'origin/master' into changes_tries_updat…

54f3d47

…e_configuration

svyatonik requested a review from mxinden as a code owner January 13, 2020 12:07

uggrade db from v0 to v1 (init CT cache + add column)

233afad

svyatonik added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jan 14, 2020

svyatonik added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 16, 2020

Merge branch 'master' into changes_tries_update_configuration

0997bd6

gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. A1-onice labels Jan 16, 2020

fix after merge

7658040

gavofyork merged commit 8e98643 into master Jan 16, 2020

gavofyork deleted the changes_tries_update_configuration branch January 16, 2020 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow updating configuration of changes tries #3201

Allow updating configuration of changes tries #3201

svyatonik commented Jul 25, 2019 •

edited

Loading

svyatonik commented Jan 9, 2020

gavofyork commented Jan 10, 2020

svyatonik commented Jan 10, 2020

Demi-Marie commented Jan 10, 2020

svyatonik commented Jan 10, 2020

gavofyork commented Jan 13, 2020

svyatonik commented Jan 13, 2020

svyatonik commented Jan 13, 2020

gavofyork commented Jan 13, 2020

svyatonik commented Jan 14, 2020

cheme commented Jan 15, 2020

svyatonik commented Jan 15, 2020

svyatonik commented Jan 16, 2020

Allow updating configuration of changes tries #3201

Allow updating configuration of changes tries #3201

Conversation

svyatonik commented Jul 25, 2019 • edited Loading

svyatonik commented Jan 9, 2020

gavofyork commented Jan 10, 2020

svyatonik commented Jan 10, 2020

Demi-Marie commented Jan 10, 2020

svyatonik commented Jan 10, 2020

gavofyork commented Jan 13, 2020

svyatonik commented Jan 13, 2020

svyatonik commented Jan 13, 2020

gavofyork commented Jan 13, 2020

svyatonik commented Jan 14, 2020

cheme commented Jan 15, 2020

svyatonik commented Jan 15, 2020

svyatonik commented Jan 16, 2020

svyatonik commented Jul 25, 2019 •

edited

Loading